Don't mutate the builder. Instead, use a local variable.

Done

Hmm... This is a bit interesting, as if the user is providing call credentials we could actually work on non-GCE.

I wonder if we are modifying the wrong API, and should be modifying GoogleDefaultChannelCredentials instead. CC @menghanl

Gax-java and google-api-go-client will check the GCP environment before attempting DirectPath, see https://github.com/googleapis/gax-java/blob/main/gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java#L336 and https://github.com/googleapis/google-api-go-client/blob/master/transport/grpc/dial.go#L141.

Hmm, or we should just add a general purpose API that accepts callCreds.
And both GoogleDefaultCreds and ComputeEngineCreds should be built on that.

What do you think of this? grpc/grpc-go#4830

Gax-java and google-api-go-client will check the GCP environment before attempting DirectPath

So what? That has little to do with the implementation and times it could be used. The point is that this API has no dependency on GCE when you provide the credentials.

And both GoogleDefaultCreds and ComputeEngineCreds should be built on that.

I think that is essentially "GoogleDefaultCreds with passed call creds" like I was suggesting. And it seems that is the approach you took with grpc/grpc-go#4830, so I think we are on the same page.

We should modify GoogleDefaultChannelCredentials in Java instead of ComputeEngineChannelCredentials.

So I have two concerns here:

1. Your opinion is from the perspective of API design, which makes sense to me. However, if you look from the perspective of DirectPath, since DirectPath can only be used on GCE, removing the checking on GCE in credentials (i.e., using GoogleDefaultCreds instead of ComputeEngineCreds) means that we relies on client libraries (i.e., the two links above) to check the GCE environment. I doubt if this is a good design, and I feel the secure solution here is to rename ComputeEngineCreds in this PR to DirectPathCreds (if ComputeEngineCreds is only used by DirectPath, and if not we can just create a new API), so that it will makes sense that the new API can take call credentials while also have dependency on GCE. WDYT? Also @menghanl

2. I remember originally we first developed GoogleDefaultCreds, but as client libraries owners wanted to know for sure what mechanism would be used to retrieve the OAuth token, so we then developed ComputeEngineCreds. @apolcyn Alex may have more context here. If after discussing Q1 we still want to use GoogleDefaultCreds for DirectPath, we should reach to client libraries owners as same concerns may still exist.

since DirectPath can only be used on GCE

For 1, as you mention, the only reason ComputeEngineChannelCredentials exists is because the client libraries wanted to convert from their version of ComputeEngine creds and guarantee that GoogleDefaultChannelCredentials didn't use some other sort of cred. GoogleDefaultChannelCredentials was designed to run anywhere.

For 2, if the client libraries are explicitly passing the call credentials they expect to be used, then there's no mis-alignment.

Agree with 2.

For 1 I want to double check, you think it is OK that DirectPath will use GoogleDefaultChannelCredentials, and since GoogleDefaultChannelCredentials does not do the on GCE check, we rely on client libraries to do the on GCE check, right?

I don't know how I can say this any differently. GoogleDefaultChannelCredentials is fine to use outside of GCE. If client libraries only want to use it for GCE, that is fine and up to them. Both GoogleDefaultChannelCredentials and ComputeEngineChannelCredentials do not require ALTS to work; they will fall back to TLS if anything goes wrong.

OK, thanks for clarifying. I have updated the PR. PTAL.

Declare private Builder() {} to favor static newBuilder() over new Builder().

done

nit: Add javadoc and @since 1.42.0

done

nit: add @since 1.42.0.

done

nit: s/Construct/Constructs/

done

nit: s/Build/Builds/

done